[SPARK-28521][SQL] Fix error message for built-in functions#25261
[SPARK-28521][SQL] Fix error message for built-in functions#25261wangyum wants to merge 4 commits intoapache:masterfrom wangyum:SPARK-28521
Conversation
|
cc @mgaido91 |
| @@ -586,8 +586,9 @@ object FunctionRegistry { | |||
| val params = Seq.fill(expressions.size)(classOf[Expression]) | |||
| val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse { | |||
There was a problem hiding this comment.
but here we're still filtering by expressions..am I missing something?
There was a problem hiding this comment.
Two reasons:
- It will throw exception at
CheckAnalysisif parameter is aDataType. So we do not need handle it here:
scala> spark.sql("select cast(int)")
org.apache.spark.sql.AnalysisException: cannot resolve '`int`' given input columns: []; line 1 pos 12;
'Project [unresolvedalias('cast('int), None)]
+- OneRowRelation
at org.apache.spark.sql.catalyst.analysis.package$AnalysisErrorAt.failAnalysis(package.scala:42)
at org.apache.spark.sql.catalyst.analysis.CheckAnalysis$$anonfun$$nestedInanonfun$checkAnalysis$1$2.applyOrElse(CheckAnalysis.scala:113)
at org.apache.spark.sql.catalyst.analysis.CheckAnalysis$$anonfun$$nestedInanonfun$checkAnalysis$1$2.applyOrElse(CheckAnalysis.scala:110)
at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$transformUp$2(TreeNode.scala:306)- It's hard to know a parameter is
DataTypeorExpressionhere.
There was a problem hiding this comment.
So, I think the problem here is not that we are not considering the DataType args, but the problem IMHO is that we are not handling the case validParametersCount == 0. I think in that case we should throw something like: Invalid arguments for function $name.. Indeed, in the cast case, I'd argue that it is not so clear to me whether the arguments are 1 or 2, since it is not cast(1, string) but cast(1 as string)... So I don't think that it is correct to state that the number of arguments for cast is 2.
|
Test build #108210 has finished for PR 25261 at commit
|
|
Test build #108438 has finished for PR 25261 at commit
|
mgaido91
left a comment
There was a problem hiding this comment.
LGTM, just a comment on wording. Thanks.
| validParametersCount.last | ||
| val expectedErrorMsg = validParametersCount.length match { | ||
| case 0 => | ||
| "" |
There was a problem hiding this comment.
nit: in case of 0, I'd prefer a more generic Invalid arguments for function $name, instead of Invalid number of arguments for function $name. Because as I mentioned earlier, I think it is questionable which is the number of arguments in this case...
# Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala
|
Test build #108500 has finished for PR 25261 at commit
|
|
Merged to master |
What changes were proposed in this pull request?
The reason is that we did not handle the case
validParametersCount.length == 0because the parameter types can beExpression,DataTypeandOption. This PR makes it handle the casevalidParametersCount.length == 0.How was this patch tested?
unit tests